VarBin: build views straight into a VarBinViewBuilder - #9127
Conversation
Merging this PR will degrade performance by 9.77%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance ChangesTip Investigate this regression by commenting Comparing Footnotes
|
30b5bcc to
d4aaedf
Compare
3cfce8e to
5f89fb3
Compare
5f89fb3 to
4a4beb0
Compare
0e6e08d to
ca493a9
Compare
onursatici
left a comment
There was a problem hiding this comment.
I wonder if we should follow up with refactoring the varbinview builder so it allows us to write views directly. This change helps us avoid re-writing the views but we still have a temporary views buffer that we copy from
|
yes, there's a cleanup pass after all the builder changes. I noticed there's patterns that could be simplified/improved but they're spread out all over |
ca493a9 to
7cab066
Compare
7cab066 to
fa6b46c
Compare
e1b2d4e to
e629da9
Compare
e629da9 to
16fb788
Compare
16fb788 to
aeb9331
Compare
Appending a `VarBin` array to a `VarBinViewBuilder` canonicalized it first, which built the views, wrapped them in a `VarBinViewArray` the builder immediately unwrapped, and then had `append_varbinview_array` rewrite every view to rebase its buffer index onto the builder's. Number the buffer up front instead: `varbin_decode_views` takes the index the pushed buffer will land at, so the views come out already correct and the append is one view per row plus a buffer push. `varbin_to_canonical` now shares that helper, which also shrinks what `match_each_integer_ptype!` duplicates. A builder configured to compact still goes the canonical route — it chooses per buffer whether to keep, slice or rewrite it by measuring the finished views, and `push_buffer_and_adjusted_views` would bypass that. chunk_array_builder, fastest of 100 samples, 3 runs each: | benchmark (rows x chunks) | before | after | speedup | | ------------------------------------- | ------- | ------- | ------- | | varbin_to_varbinview_builder 10x1000 | 313 µs | 204 µs | 1.53x | | varbin_to_varbinview_builder 100x100 | 87.3 µs | 69.5 µs | 1.26x | | varbin_to_varbinview_builder 1000x10 | 58.6 µs | 54.8 µs | 1.07x | | varbin_opt_to_varbinview_bldr 10x1000 | 378 µs | 266 µs | 1.42x | | varbin_into_canonical 10x1000 | 357 µs | 253 µs | 1.41x | The last row is the control: it never touches the builder, so its gain is from the shared-helper extraction alone. Relative to it, the append itself drops from 0.88x to 0.81x of a canonicalization. Signed-off-by: Robert Kruszewski <github@robertk.io>
aeb9331 to
9ba66f9
Compare
Instead of requiring canonical for varbin to export to varbinview builder we export directly to varbinview builder